fix: prevent iterator invalidation in handleDeadDefender (BUG-02)#3943
fix: prevent iterator invalidation in handleDeadDefender (BUG-02)#3943berkelmali wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
WalkthroughThe change snapshots the defender's tiles each iteration in ChangesPost-defender conquest sweep
Sequence Diagram(s)sequenceDiagram
participant AttackExecution
participant Target
participant Tile
AttackExecution->>Target: snapshot = Array.from(target.tiles())
loop per snapshot (<=100)
AttackExecution->>Tile: evaluate border vs interior
AttackExecution->>Tile: this._owner.conquer(tile)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/AttackExecution.ts`:
- Around line 370-373: Add deterministic regression tests for the new sweep
iteration logic in AttackExecution (the loop that calls target.tiles() and does
up to 100 passes). Create tests that (1) mutate the live collection during
iteration to verify safe snapshot behavior by asserting no runtime errors and
that mutated tiles are processed as expected, (2) simulate a larger
dead-defender tile set to confirm full absorption (all defender tiles removed)
after the sweep completes, and (3) verify stable termination when no tiles
remain by asserting the loop exits early (fewer than 100 passes) and no infinite
loop occurs; use a fixed board/state or seeded RNG to ensure determinism and
reference the AttackExecution class and its sweep behavior (the for-loop using
target.tiles()) in test names and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dadb6691-141d-4eea-93f3-a13671286c01
📒 Files selected for processing (1)
src/core/execution/AttackExecution.ts
| for (let i = 0; i < 100; i++) { | ||
| const remainingTiles = Array.from(target.tiles()); | ||
| if (remainingTiles.length === 0) break; | ||
| for (const tile of remainingTiles) { |
There was a problem hiding this comment.
Add regression tests for this core simulation change
This fixes a real runtime edge case, but the PR includes no tests for the new sweep behavior (snapshot iteration + 100-pass/early-break flow). Please add deterministic tests that cover at least: (1) live-collection mutation safety, (2) full absorption for larger dead-defender tile sets, and (3) stable termination when no tiles remain.
As per coding guidelines: “All changes to src/core/ must include tests”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/execution/AttackExecution.ts` around lines 370 - 373, Add
deterministic regression tests for the new sweep iteration logic in
AttackExecution (the loop that calls target.tiles() and does up to 100 passes).
Create tests that (1) mutate the live collection during iteration to verify safe
snapshot behavior by asserting no runtime errors and that mutated tiles are
processed as expected, (2) simulate a larger dead-defender tile set to confirm
full absorption (all defender tiles removed) after the sweep completes, and (3)
verify stable termination when no tiles remain by asserting the loop exits early
(fewer than 100 passes) and no infinite loop occurs; use a fixed board/state or
seeded RNG to ensure determinism and reference the AttackExecution class and its
sweep behavior (the for-loop using target.tiles()) in test names and assertions.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/HandleDeadDefender.test.ts`:
- Line 141: Capture the attacker's tile count before performing the sweep/attack
by storing attacker.numTilesOwned() in a variable (e.g., preTiles), then run the
existing sweep/attack call in the test, then get a new count (postTiles =
attacker.numTilesOwned()) and replace the current loose assertion
expect(attacker.numTilesOwned()).toBeGreaterThan(0) with
expect(postTiles).toBeGreaterThan(preTiles) to assert tiles were transferred
from the defender; use variable names like preTiles/postTiles to make intent
clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1dcb017-7bd9-43d3-aab8-e5e6a2973fd0
📒 Files selected for processing (1)
tests/HandleDeadDefender.test.ts
| expect(defender.numTilesOwned()).toBe(0); | ||
|
|
||
| // Attacker should have gained tiles | ||
| expect(attacker.numTilesOwned()).toBeGreaterThan(0); |
There was a problem hiding this comment.
Strengthen the assertion to verify tile transfer.
The assertion checks that the attacker has more than zero tiles, but the attacker already owns tiles from spawning (line 48-50). This doesn't prove that tiles were transferred from the defender during the sweep.
Consider capturing the attacker's tile count before the attack and asserting that it increased:
💪 Suggested improvement
+ const attackerTilesBefore = attacker.numTilesOwned();
+
// Attack and kill the defender
game.addExecution(new AttackExecution(null, attacker, defender.id(), null));
for (let i = 0; i < 500; i++) {
game.executeNextTick();
if (!defender.isAlive()) break;
}
// All defender tiles should now belong to someone other than defender
expect(defender.numTilesOwned()).toBe(0);
// Attacker should have gained tiles
- expect(attacker.numTilesOwned()).toBeGreaterThan(0);
+ expect(attacker.numTilesOwned()).toBeGreaterThan(attackerTilesBefore);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(attacker.numTilesOwned()).toBeGreaterThan(0); | |
| const attackerTilesBefore = attacker.numTilesOwned(); | |
| // Attack and kill the defender | |
| game.addExecution(new AttackExecution(null, attacker, defender.id(), null)); | |
| for (let i = 0; i < 500; i++) { | |
| game.executeNextTick(); | |
| if (!defender.isAlive()) break; | |
| } | |
| // All defender tiles should now belong to someone other than defender | |
| expect(defender.numTilesOwned()).toBe(0); | |
| // Attacker should have gained tiles | |
| expect(attacker.numTilesOwned()).toBeGreaterThan(attackerTilesBefore); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/HandleDeadDefender.test.ts` at line 141, Capture the attacker's tile
count before performing the sweep/attack by storing attacker.numTilesOwned() in
a variable (e.g., preTiles), then run the existing sweep/attack call in the
test, then get a new count (postTiles = attacker.numTilesOwned()) and replace
the current loose assertion expect(attacker.numTilesOwned()).toBeGreaterThan(0)
with expect(postTiles).toBeGreaterThan(preTiles) to assert tiles were
transferred from the defender; use variable names like preTiles/postTiles to
make intent clear.
berkelmali
force-pushed
the
fix/bug02-dead-defender-iterator
branch
from
b73fe47 to
e3ffe0d
Compare
Snapshot target.tiles() into an array before iterating to prevent modifying the collection during iteration when conquer() is called. Also increase loop limit from 10 to 100 with early break on empty to properly handle large empires.
berkelmali
force-pushed
the
fix/bug02-dead-defender-iterator
branch
from
e3ffe0d to
576544c
Compare
1 participant
Resolves #4091
Description:
In
AttackExecution.handleDeadDefender(),target.tiles()returns a liveSet<TileRef>that is mutated mid-iteration whenconquer(tile)is called. Per ECMA-262, deleted entries are silently skipped by the iterator, causing some defender tiles to be missed. Those tiles become orphaned phantom territories owned by a dead player. On the next game tick, downstream systems attempt to call methods on the stale owner reference, throwing an uncaughtTypeErrorthat crashes the server.This PR snapshots the tile collection with
Array.from(target.tiles())before iterating, eliminating iterator invalidation. The inner loop limit is increased from 10 to 100 with an earlybreakwhen no tiles remain, ensuring large empires are fully absorbed.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires